Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.86%. Comparing base (80bb5bf) to head (1d22c51).

❗ Current head 1d22c51 differs from pull request most recent head be2e9d2. Consider uploading reports for the commit be2e9d2 to get more accurate results

@@            Coverage Diff             @@
##             main    #4136      +/-   ##
==========================================
+ Coverage   89.49%   89.86%   +0.37%     
==========================================
  Files         431      432       +1     
  Lines       19555    19411     -144     
==========================================
- Hits        17500    17443      -57     
+ Misses       2055     1968      -87     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I think having the cancel state projected back to the computeVelocityCommands as an argument would be important. Right now, I don't know how the controller would know when cancel stopped and a new request came in otherwise. So I think cancelling should be given to it.

Or possibly even have separate computeCancelCommands() API to use when cancelling = true. That may be a more ergonomics thing: I imagine that when you have a controller that is handling normal vs cancel state, you're basically having an if/else for 2 very different methods either way. Those would probably render into separate functions, so we could just call separate functions. We could have the if/else in computeAndPublishVelocity with new arg cancelling

Then, the if (controllers_[current_controller_]->cancel()) { check woudl be more like cancelled() to indicate that its completed its cancellation to allow us to action_server_->terminate_all(); and publishZeroVelocity(); for good measure (and where the timeout is located)

So all in all, I like your design, but I think we an do it a little bit better with addresses the problem of the controller knowing when we're back outside of the non-canceled state for normal operations & adding a timeout in case a plugin isn't properly implemented

@SteveMacenski which controller is the easiest to modify with this new API? I was thinking about the pure pursuit controller but that one doesn't have decelleration limits so I don't know how fast to brake. How is the acceleration determined?

Which ever one you like - I think DWB is the only one with explicit decel/accel parameters right now. MPPI will have it before EOY, but not its current state. RPP is a pure geometric approach, so there's not really any dynamic modeling (right, its just following a carrot). We have some constraints on speed based on some heuristic functions, but that's also not dynamics based, its situational.

@Rayman, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@Rayman anything I need to review? Or still working on it 😄

@Rayman, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

Yes sorry I'm still working on it.

I've modified the RPP controller to add a cancel_decelleration parameter that shows how to use the new API. Is this in line with the design of the controller? I thought that one was the easiest to modify. Most controllers don't have acceleration limits, only the DWB. But that one is quite complicated with the critics so I didn't know how to modify it to support cancellation.

I've also adressed one of your review comments. Last thing to do is add some unit tests probably.

@Rayman, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

Generally looks good to me!

@Rayman, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

I don't understand the CI failure. I didn't edit any package.xml so why is it complaining about dependency issues?

@Rayman we're in a weird state due to the rolling transition to 24.04. I'll plan to look into it in more detail on Friday, but for now, if you tell me it works and you tested it, I'd trust that assuming the other checks look good

@Rayman, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

I've tested this only in simulation for now. I'll test this on a robot in the next coming weeks but I need some more parts to be ready for that. I'll send this PR also to a collegue to review

Sim is good enough for me - yal know what you're doing. Feel free to implement other controllers if you like (or contribute your controller here) 😉

Thanks @Rayman !

Nice work, I haven't tested it but reading on the changes, 2 questions:

1. In the way it is implemented in RPP, I believe it is now a mandatory behavior to use the cancel_deceleration. Any way to making it optional, for instance when one wants to rely on the deceleration configured and applied in a velocity smoother node (and without having to manage another decel limit parameter somewhere else?
2. Is it applied when preempting the controller server action ? (in my current use-case I would not want that, but I can think of use cases where it would be desirable)

I believe it is now a mandatory behavior to use the cancel_deceleration

You could set the deceleration to something insanely high to in effect disable, but this is a good point, it should be an option. We can make a param pretty easily that'll bypass this if needed. I filed a ticket #4238 for record keeping

Is it applied when preempting the controller server action

Nope, only on cancel:

      if (action_server_->is_cancel_requested()) {
        if (controllers_[current_controller_]->cancel()) {
          ...
        } ...
      }

All good short term then!